Skip to content

Fix false positives for unused_enumerated with result member access#6544

Open
theamodhshetty wants to merge 3 commits intorealm:mainfrom
theamodhshetty:codex/fix-unused-enumerated-result-member
Open

Fix false positives for unused_enumerated with result member access#6544
theamodhshetty wants to merge 3 commits intorealm:mainfrom
theamodhshetty:codex/fix-unused-enumerated-result-member

Conversation

@theamodhshetty
Copy link
Copy Markdown
Contributor

@theamodhshetty theamodhshetty commented Mar 17, 2026

Summary

  • avoid flagging .enumerated() when a higher-order call uses tuple members like ?.offset after the closure
  • keep tracking $0.element / $0.0 and seed the closure usage from result-member access on the surrounding call
  • add regression examples and the required changelog entry

Testing

  • swift test --filter UnusedEnumeratedRuleGeneratedTests
  • ./.build/debug/swiftlint lint Source/SwiftLintBuiltInRules/Rules/Idiomatic/UnusedEnumeratedRule.swift CHANGELOG.md --strict
  • ./.build/debug/swiftlint lint --only-rule unused_enumerated /tmp/unused_enumerated_ok.swift --quiet
  • ./.build/debug/swiftlint lint --only-rule unused_enumerated /tmp/unused_enumerated_bad.swift --quiet

Closes #5881

@theamodhshetty theamodhshetty mentioned this pull request Mar 17, 2026
Open
2 tasks
@SwiftLintBot
Copy link
Copy Markdown

SwiftLintBot commented Mar 17, 2026 •
edited
Loading

1 Warning
⚠️ This PR may need tests.
19 Messages
📖 Building this branch resulted in a binary size of 27359.23 KiB vs 27357.74 KiB when built on main (0% larger).
📖 Linting Aerial with this PR took 0.18 s vs 0.15 s on main (20% slower).
📖 Linting Alamofire with this PR took 0.2 s vs 0.17 s on main (17% slower).
📖 Linting Brave with this PR took 0.86 s vs 0.86 s on main (0% slower).
📖 Linting DuckDuckGo with this PR took 3.51 s vs 3.52 s on main (0% faster).
📖 Linting Firefox with this PR took 1.57 s vs 1.57 s on main (0% slower).
📖 Linting Kickstarter with this PR took 0.97 s vs 0.99 s on main (2% faster).
📖 Linting Moya with this PR took 0.1 s vs 0.13 s on main (23% faster).
📖 Linting NetNewsWire with this PR took 0.33 s vs 0.33 s on main (0% slower).
📖 Linting Nimble with this PR took 0.18 s vs 0.15 s on main (20% slower).
📖 Linting PocketCasts with this PR took 0.94 s vs 0.93 s on main (1% slower).
📖 Linting Quick with this PR took 0.13 s vs 0.12 s on main (8% slower).
📖 Linting Realm with this PR took 0.39 s vs 0.4 s on main (2% faster).
📖 Linting Sourcery with this PR took 0.33 s vs 0.33 s on main (0% slower).
📖 Linting Swift with this PR took 0.47 s vs 0.49 s on main (4% faster).
📖 Linting SwiftLintPerformanceTests with this PR took 3.86 s vs 3.82 s on main (1% slower).
📖 Linting VLC with this PR took 0.27 s vs 0.26 s on main (3% slower).
📖 Linting Wire with this PR took 2.05 s vs 2.06 s on main (0% faster).
📖 Linting WordPress with this PR took 1.38 s vs 1.39 s on main (0% faster).

Generated by 🚫 Danger

@theamodhshetty
Copy link
Copy Markdown
Contributor Author

theamodhshetty commented Mar 17, 2026

Pushed a small follow-up in eb3575a after the first CI pass.

The original version reset nextClosureId / lastEnumeratedPosition inside the same if let that introduced shorthand bindings with those names, which newer matrix legs were treating as the shadowed locals instead of the stored properties. This update just renames the local bindings so the property resets stay unambiguous across toolchains.

@rgoldberg
Copy link
Copy Markdown

rgoldberg commented Apr 5, 2026

Does anything else need to be done to merge this PR?

@theamodhshetty
Copy link
Copy Markdown
Contributor Author

theamodhshetty commented Apr 5, 2026

I don’t think so from my side. The follow-up for the CI/toolchain issue is already in, and the checks are green now. If there’s anything else you want adjusted before merge, I’m happy to make it.

SimplyDanny
SimplyDanny requested changes Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

The idea looks good. I have a few remarks though ...

@theamodhshetty
Copy link
Copy Markdown
Contributor Author

theamodhshetty commented Apr 9, 2026

Pushed a follow-up in 7ba4ed1 that addresses the review points from SimplyDanny.

Changes in this pass:

  • collapsed the parentCall / trailingClosure setup into the cleaner form from the suggestions
  • reworked the closure tracking so only tracked closures carry an enumeratedPosition, instead of using a nullable position on every stack entry
  • tightened the result-member ancestor walk so it stops once we leave the optional/member-access chain, while still continuing past unrelated member names until it can see a later offset / element

Validation rerun:

  • swift test --filter UnusedEnumeratedRuleGeneratedTests
  • ./.build/debug/swiftlint lint Source/SwiftLintBuiltInRules/Rules/Idiomatic/UnusedEnumeratedRule.swift --strict

If you want this split up differently, I can adjust it.

SimplyDanny
SimplyDanny reviewed Apr 9, 2026
View reviewed changes
Source/SwiftLintBuiltInRules/Rules/Idiomatic/UnusedEnumeratedRule.swift
Comment on lines +239 to +253
private var currentTrackedClosure: Closure? {
closures.peek().flatMap(\.self)
}

private func popTrackedClosure() -> Closure? {
closures.pop().flatMap(\.self)
}

private func modifyTrackedClosure(_ modifier: (inout Closure) -> Void) {
closures.modifyLast {
guard var closure = $0 else { return }
modifier(&closure)
$0 = closure
}
}
Copy link
Copy Markdown
Collaborator

@SimplyDanny SimplyDanny Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to inline them as each of them is only used once.

Source/SwiftLintBuiltInRules/Rules/Idiomatic/UnusedEnumeratedRule.swift

guard parent.is(OptionalChainingExprSyntax.self)
|| parent.is(ForceUnwrapExprSyntax.self)
|| parent.is(MemberAccessExprSyntax.self)
Copy link
Copy Markdown
Collaborator

@SimplyDanny SimplyDanny Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also consider tuple expressions with single elements (i.e. parenthesized expressions).

Source/SwiftLintBuiltInRules/Rules/Idiomatic/UnusedEnumeratedRule.swift
&& arguments.isEmpty
}

var usedEnumeratedResultMembers: (zero: Bool, one: Bool) {
Copy link
Copy Markdown
Collaborator

@SimplyDanny SimplyDanny Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may attach this to ExprSyntax instead. So we wouldn't need the while at all and can rely on recursion with the types listed in 319.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SimplyDanny SimplyDanny SimplyDanny requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive for unused_enumerated

4 participants

@theamodhshetty @SwiftLintBot @rgoldberg @SimplyDanny